perf(cosmetics): optimize 7TV cosmetics performance #47
Conversation
Replace O(n) .find() operations with O(1) Map.get() lookups for badges and paints. This eliminates performance lag when rendering messages with 7TV cosmetics by: - Adding cosmeticsLookup state with badgeMap and paintMap hashmaps - Populating lookup maps in addCosmetics() when cosmetics are loaded - Replacing .find() calls with Map.get() in getUserStyle, getUserBadge, getUserPaint Fixes chat lag after recent 7TV styling bug fix by transforming complexity from O(n × messages) to O(1 × messages) for cosmetic lookups.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRefactors cosmetics into O(1) lookup maps with add/remove handlers, routes and deduplicates 7TV events via a shared websocket with per-chatroom subscription updates, instruments console.warn/error for telemetry, adds progressive emote loading and styles, and removes the legacy StvWebSocket module. Changes
Sequence Diagram(s)sequenceDiagram
participant SharedWS as SharedStvWebSocket
participant ChatProv as ChatProvider
participant CosmeticsProv as CosmeticsProvider
participant Store as App State
participant EmoteComp as Emote Component
participant Telemetry as Telemetry
rect rgba(230,240,255,0.25)
SharedWS->>ChatProv: emit event (cosmetic.create / cosmetic.delete / emote_set.update / entitlement.*)
ChatProv->>ChatProv: dedupe (recentCosmeticEvents / recentPersonalUpdates / recentChannelUpdates)
ChatProv->>CosmeticsProv: dispatch (addCosmetic / removeCosmetic / addCosmetics / emote_set.update)
CosmeticsProv->>CosmeticsProv: normalize → badgeMap / paintMap (cosmeticsLookup)
CosmeticsProv->>Store: update cosmetics + cosmeticsLookup
end
rect rgba(240,255,230,0.25)
ChatProv->>SharedWS: updateChatroom(chatroomId, ...)
SharedWS->>SharedWS: manage subscriptions (unsubscribe old → subscribe new)
SharedWS-->>ChatProv: events / confirmations (with isPersonalEmoteSet)
end
rect rgba(255,245,230,0.25)
EmoteComp->>CosmeticsProv: resolve badge/paint via cosmeticsLookup
EmoteComp->>EmoteComp: progressive loading lifecycle (placeholder → loading → loaded/error)
end
rect rgba(250,250,255,0.15)
ChatProv->>Telemetry: record spans/metrics (counts & durations)
Telemetry-->>ChatProv: ack
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/renderer/src/providers/CosmeticsProvider.jsx (3)
93-95: Normalize misses to null to avoid undefined leaks.Coalesce Map.get() results to null for consistent return types.
- const { badgeMap, paintMap } = get().cosmeticsLookup; - const badge = userStyle.badgeId ? badgeMap.get(userStyle.badgeId) : null; - const paint = userStyle.paintId ? paintMap.get(userStyle.paintId) : null; + const { badgeMap, paintMap } = get().cosmeticsLookup; + const badge = userStyle.badgeId ? (badgeMap.get(userStyle.badgeId) ?? null) : null; + const paint = userStyle.paintId ? (paintMap.get(userStyle.paintId) ?? null) : null;
137-144: Add username guard and null‑coalesce badge lookup.Prevents toLowerCase on undefined and normalizes return to null.
getUserBadge: (username) => { - const transformedUsername = username.toLowerCase(); + if (!username) return null; + const transformedUsername = username.toLowerCase(); const userStyle = get().userStyles[transformedUsername]; if (!userStyle?.badgeId) return null; - return get().cosmeticsLookup.badgeMap.get(userStyle.badgeId); + return get().cosmeticsLookup.badgeMap.get(userStyle.badgeId) ?? null; },
146-153: Add username guard and null‑coalesce paint lookup.Same rationale as badge.
getUserPaint: (username) => { - const transformedUsername = username.toLowerCase(); + if (!username) return null; + const transformedUsername = username.toLowerCase(); const userStyle = get().userStyles[transformedUsername]; if (!userStyle?.paintId) return null; - return get().cosmeticsLookup.paintMap.get(userStyle.paintId); + return get().cosmeticsLookup.paintMap.get(userStyle.paintId) ?? null; },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/renderer/src/providers/CosmeticsProvider.jsx(5 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**
📄 CodeRabbit inference engine (AGENTS.md)
**: Use electron-vite + React conventions instead of raw Electron patterns
When unsure, consult electron-vite/Electron/Vite docs (Context7) to confirm patterns
Omit empty sections in release notes
Files:
src/renderer/src/providers/CosmeticsProvider.jsx
src/renderer/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/renderer/src/**/*.{ts,tsx,js,jsx}: Do not use direct Node APIs in the renderer
Use ipcRenderer.invoke('channel', payload) for request/response from renderer
Use import.meta.env.RENDERER_VITE_* for renderer-scoped config
Avoid bare KT_* env vars in the renderer; use RENDERER_VITE_KT_* instead
Files:
src/renderer/src/providers/CosmeticsProvider.jsx
src/{renderer/src,preload}/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Resolve static assets via Vite URLs (new URL('./asset', import.meta.url))
Files:
src/renderer/src/providers/CosmeticsProvider.jsx
src/{main,renderer/src}/**/*.{ts,tsx,js,jsx,mts,mjs,cjs}
📄 CodeRabbit inference engine (AGENTS.md)
For fire-and-forget events, use ipcRenderer.send and ipcMain.on with namespaced channels like 'app:settings:get'
Files:
src/renderer/src/providers/CosmeticsProvider.jsx
src/{main,preload,renderer/src}/**/*.{ts,tsx,js,jsx,mts,mjs,cjs}
📄 CodeRabbit inference engine (AGENTS.md)
Use shared non-secret variables via import.meta.env.VITE_*
Files:
src/renderer/src/providers/CosmeticsProvider.jsx
!dist/**
📄 CodeRabbit inference engine (AGENTS.md)
Do not commit built installers/artifacts in dist/
Files:
src/renderer/src/providers/CosmeticsProvider.jsx
🧬 Code graph analysis (1)
src/renderer/src/providers/CosmeticsProvider.jsx (2)
utils/services/seventv/sharedStvWebSocket.js (1)
body(17-17)utils/services/seventv/stvWebsocket.js (1)
body(13-13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test-builds (windows-latest)
🔇 Additional comments (3)
src/renderer/src/providers/CosmeticsProvider.jsx (3)
9-12: Good addition: O(1) lookup maps (badge/paint). Confirm no persistence needed for Map.Looks solid for performance. If this store is ever persisted (zustand/persist), Map won’t serialize without custom transforms.
Do we persist this store? If yes, we should add persist transforms or switch to plain objects for the maps.
127-130: LGTM: State update merges maps without replacing other keys.Shallow merging via set(() => newState) is correct here; userStyles remains intact.
107-122: Build lookup maps defensively; confirm addCosmetics receives full snapshots
- Replace the forEach-based map build with Array.isArray + Map constructor (suggested diff):
- // Create lookup maps for O(1) access - const badgeMap = new Map(); - const paintMap = new Map(); - - if (body.badges) { - body.badges.forEach(badge => { - badgeMap.set(badge.id, badge); - }); - } - - if (body.paints) { - body.paints.forEach(paint => { - paintMap.set(paint.id, paint); - }); - } + // Create lookup maps for O(1) access + const badgeMap = new Map((Array.isArray(body.badges) ? body.badges : []).map(b => [b.id, b])); + const paintMap = new Map((Array.isArray(body.paints) ? body.paints : []).map(p => [p.id, p]));
- Confirm event semantics: addCosmetics currently replaces globalCosmetics and cosmeticsLookup — if body can be a delta rather than a full snapshot, this will drop prior entries and must be handled differently.
- Remaining O(n) scans found; update callsites to use the lookup maps where possible: utils/services/seventv/stvWebsocket.js (lines ~19–21, ~39–41, ~132–134) and utils/services/seventv/sharedStvWebSocket.js (lines ~23–25, ~41–43) use cosmetics.badges.find / cosmetics.paints.find.
…t handling Major refactoring to improve 7TV WebSocket performance and fix cosmetic event processing: **Shared WebSocket Migration:** - Replace per-chatroom 7TV WebSocket connections with single shared connection - Remove individual StvWebSocket class (423 lines deleted) - Update sharedStvWebSocket with proper event routing and subscription management - Deprecate connectToStvWebSocket() method in ChatProvider - Update ConnectionManager to use updateChatroom() instead of addChatroom() **Entitlement Event Handling:** - Fix cosmetic event deduplication using ref_id instead of all-zeros id field - Remove kind === 10 filter that was blocking EMOTE_SET entitlements - Route BADGE/PAINT events to cosmetics store, EMOTE_SET to dedicated handler - Add handlePersonalEmoteSetEntitlement() placeholder for future implementation - Handle global cosmetic events once instead of broadcasting to all chatrooms **Performance Improvements:** - Add deduplication for cosmetic events (30s window, 60s cleanup) - Add dedicated tracking for personal and channel emote set updates - Reduce console log spam by 82% (2900+ → 500 lines) - Add VITE_DEBUG_7TV_WS flag for optional verbose WebSocket logging **Telemetry Enhancements:** - Add console.error/console.warn instrumentation in webTracing - Capture critical warnings and errors for telemetry **Related:** - Issue #48: Full implementation needed for personal emote set entitlement sync - Builds on commit 93f2790 (hashmap optimization for cosmetics lookups) This refactor eliminates redundant WebSocket connections while fixing several bugs in how 7TV cosmetic and entitlement events are processed.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.env.example (1)
94-97: Consider addressing minor style conventions.Static analysis flagged two style conventions:
- Variable ordering:
MAIN_VITE_GRAFANA_TEMPO_QUERY_TOKENcould be placed beforeMAIN_VITE_GRAFANA_TEMPO_QUERY_URLfor alphabetical consistency.- Missing trailing blank line at end of file (common convention).
Apply this diff to address both conventions:
MAIN_VITE_GRAFANA_TEMPO_QUERY_URL=https://tempo-prod-xx-prod-us-east-x.grafana.net/tempo MAIN_VITE_GRAFANA_TEMPO_QUERY_USER=your_grafana_user_id MAIN_VITE_GRAFANA_TEMPO_QUERY_TOKEN=glc_your_grafana_cloud_access_policy_token_here # Debug flags (disable by default, enable for troubleshooting) # VITE_DEBUG_7TV_WS=true # Enable verbose 7TV WebSocket message logging +src/renderer/src/providers/ChatProvider.jsx (1)
3363-3388: Stub properly documents future implementation needs.The stub implementation clearly documents the intended behavior with a TODO comment and explains the distinction between current user and other users' personal emote sets. The reference to GitHub issue #48 helps track the completion of this feature.
Do you want me to generate a detailed implementation plan or open a tracking issue for completing the personal emote set entitlement handler?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.env.example(1 hunks)src/renderer/src/providers/ChatProvider.jsx(17 hunks)src/renderer/src/telemetry/webTracing.js(1 hunks)utils/services/connectionManager.js(1 hunks)utils/services/seventv/sharedStvWebSocket.js(7 hunks)utils/services/seventv/stvAPI.js(1 hunks)utils/services/seventv/stvWebsocket.js(0 hunks)
💤 Files with no reviewable changes (1)
- utils/services/seventv/stvWebsocket.js
🧰 Additional context used
📓 Path-based instructions (7)
**
📄 CodeRabbit inference engine (AGENTS.md)
**: Use electron-vite + React conventions instead of raw Electron patterns
When unsure, consult electron-vite/Electron/Vite docs (Context7) to confirm patterns
Omit empty sections in release notes
Files:
utils/services/seventv/stvAPI.jssrc/renderer/src/providers/ChatProvider.jsxutils/services/seventv/sharedStvWebSocket.jssrc/renderer/src/telemetry/webTracing.jsutils/services/connectionManager.js
!dist/**
📄 CodeRabbit inference engine (AGENTS.md)
Do not commit built installers/artifacts in dist/
Files:
utils/services/seventv/stvAPI.jssrc/renderer/src/providers/ChatProvider.jsxutils/services/seventv/sharedStvWebSocket.jssrc/renderer/src/telemetry/webTracing.jsutils/services/connectionManager.js.env.example
src/renderer/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/renderer/src/**/*.{ts,tsx,js,jsx}: Do not use direct Node APIs in the renderer
Use ipcRenderer.invoke('channel', payload) for request/response from renderer
Use import.meta.env.RENDERER_VITE_* for renderer-scoped config
Avoid bare KT_* env vars in the renderer; use RENDERER_VITE_KT_* instead
Files:
src/renderer/src/providers/ChatProvider.jsxsrc/renderer/src/telemetry/webTracing.js
src/{renderer/src,preload}/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Resolve static assets via Vite URLs (new URL('./asset', import.meta.url))
Files:
src/renderer/src/providers/ChatProvider.jsxsrc/renderer/src/telemetry/webTracing.js
src/{main,renderer/src}/**/*.{ts,tsx,js,jsx,mts,mjs,cjs}
📄 CodeRabbit inference engine (AGENTS.md)
For fire-and-forget events, use ipcRenderer.send and ipcMain.on with namespaced channels like 'app:settings:get'
Files:
src/renderer/src/providers/ChatProvider.jsxsrc/renderer/src/telemetry/webTracing.js
src/{main,preload,renderer/src}/**/*.{ts,tsx,js,jsx,mts,mjs,cjs}
📄 CodeRabbit inference engine (AGENTS.md)
Use shared non-secret variables via import.meta.env.VITE_*
Files:
src/renderer/src/providers/ChatProvider.jsxsrc/renderer/src/telemetry/webTracing.js
src/{telemetry,renderer/src/telemetry}/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/{telemetry,renderer/src/telemetry}/**/*.{ts,tsx,js,jsx}: Place OpenTelemetry instrumentation under src/telemetry/** and src/renderer/src/telemetry/**
Configure OTLP HTTP export to Grafana Cloud; read endpoints/keys from env with main-safe prefixes; do not expose secrets to renderer
Files:
src/renderer/src/telemetry/webTracing.js
🧬 Code graph analysis (2)
src/renderer/src/providers/ChatProvider.jsx (4)
utils/services/seventv/stvAPI.js (1)
sendUserPresence(100-127)utils/services/seventv/sharedStvWebSocket.js (1)
body(17-17)src/renderer/src/providers/CosmeticsProvider.jsx (1)
useCosmeticsStore(3-154)src/preload/index.js (1)
personalEmoteSets(112-112)
src/renderer/src/telemetry/webTracing.js (3)
src/telemetry/tracing.js (1)
telemetryEnabled(12-12)src/telemetry/error-monitoring.js (1)
errorMessage(315-315)src/renderer/src/main.jsx (1)
error(53-53)
🪛 dotenv-linter (3.3.0)
.env.example
[warning] 94-94: [UnorderedKey] The MAIN_VITE_GRAFANA_TEMPO_QUERY_TOKEN key should go before the MAIN_VITE_GRAFANA_TEMPO_QUERY_URL key
(UnorderedKey)
[warning] 97-97: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test-builds (windows-latest)
🔇 Additional comments (15)
.env.example (1)
96-97: LGTM! Debug flag addition aligns with 7TV optimization work.The new debug flags section with
VITE_DEBUG_7TV_WSis appropriate for troubleshooting the 7TV WebSocket optimizations introduced in this PR. Good practice to have it disabled by default.utils/services/seventv/sharedStvWebSocket.js (4)
196-253: LGTM! Well-structured chatroom update handling.The
updateChatroommethod correctly handles both new chatroom additions and emote set updates. The unsubscribe/subscribe logic for changed emote sets is sound, and the validation againstINVALID_7TV_NULL_IDprevents invalid subscriptions.
712-725: LGTM! Improved flexibility for entitlement event handling.Moving the filtering logic to ChatProvider allows for more flexible handling of different entitlement kinds (BADGE, PAINT, EMOTE_SET) as mentioned in the comment. This aligns well with the centralized event handling approach in this PR.
402-404: LGTM! Useful subscription lifecycle logging.The added logging provides clear visibility into the subscription setup process and helps diagnose potential connection issues.
733-794: All findChatroomForEvent callers use the updated (type, body) signature.src/renderer/src/providers/ChatProvider.jsx (10)
390-392: LGTM! Deduplication structures properly initialized.The global deduplication structures are well-designed: Sets for simple presence checks (personal/channel updates) and a Map for timestamp-based deduplication (cosmetic events).
858-862: LGTM! Clear deprecation marker.The commented-out method with a clear deprecation notice helps prevent accidental usage while documenting the migration to shared connections.
1811-1839: LGTM! Effective cosmetic event deduplication.The deduplication logic correctly uses
refIdfrom entitlements (which represents the actual badge/paint/emote_set ID) instead of the event ID. The 30-second deduplication window with 60-second cleanup is reasonable for handling duplicate WebSocket events.
1419-1439: Excellent optimization for cosmetic event handling.Processing cosmetic events globally once (with
chatroomId: null) instead of broadcasting to all chatrooms eliminates significant duplicate work. This directly addresses the performance goals mentioned in the PR objectives.
1850-1876: LGTM! Well-structured entitlement handling by kind.The filtering by
objectKindprovides clear separation between EMOTE_SET, BADGE, and PAINT entitlements. The username normalization (replaceAll("-", "_")) ensures consistency with the cosmetics store.
2886-2917: LGTM! Effective personal emote set deduplication.The personal emote set detection and early return prevents duplicate processing across multiple chatrooms. The deduplication using a timestamp-based key (limited to last 100 entries) is a good balance between preventing duplicates and avoiding memory growth.
2937-2986: Verify aggressive deduplication doesn't suppress legitimate updates.The channel update deduplication uses both a detailed key (including pulled/pushed/updated content) and a simpler time-based key. The simpler deduplication has a 5-second window and only applies when the emote set exists in tracked chatrooms. Ensure this doesn't suppress rapid legitimate updates (e.g., multiple emotes added in quick succession).
Consider if the 5-second window is appropriate for your use case. If emotes are frequently updated in rapid succession, this could suppress valid updates.
3192-3361: LGTM! Comprehensive personal emote set handling.The method correctly processes all three types of emote updates (pulled/pushed/updated) and maintains global state. The telemetry tracking is helpful for monitoring the performance impact of personal emote set updates.
540-564: LGTM! Return value enables better tracking.The method now returns a timestamp when presence updates are sent successfully, which enables the caller to track which chatrooms had updates sent. Returning
nullfor skipped updates is appropriate.
4218-4243: LGTM! Well-designed presence update batching.Processing presence updates in small batches (size 3) with 1ms delays between batches prevents blocking the main thread while still ensuring all updates complete relatively quickly. The simplified logging that only occurs after all batches complete reduces console noise.
Remove synthetic test emissions that were flooding production telemetry: - Remove setTimeout block that emitted synthetic console.error after 2 seconds - Gate renderer_export_smoke test span behind NODE_ENV === 'development' check - Document remaining test code as DEV-ONLY These test blocks were useful during development but should not run in production as they create false positives in telemetry data.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/renderer/src/telemetry/webTracing.js (2)
200-229: Stack trace will not reflect the original caller.The stack capture on line 216 creates a new
Error()inside the wrapper, so the stack trace will start from this instrumentation code rather than the original caller ofconsole.error. This reduces debugging utility since developers won't see the call site that triggered the error.Consider capturing the stack before invoking
originalConsoleError:console.error = function(...args) { + // Capture stack before calling original to preserve call site + const stack = (new Error()).stack; + // Call original console.error first originalConsoleError.apply(console, args); // Send to telemetry try { // Create error message from arguments const errorMessage = args.map(arg => typeof arg === 'string' ? arg : arg instanceof Error ? arg.message : JSON.stringify(arg) ).join(' '); // Send to telemetry if available if (typeof window.app?.telemetry?.recordError === 'function') { const error = new Error(errorMessage); - error.stack = (new Error()).stack; // Get current stack + error.stack = stack; // Use pre-captured stack window.app.telemetry.recordError(error, { source: 'console.error', operation: 'console_error_capture', arguments: args.length, timestamp: Date.now() }); } } catch (telemetryError) { // Don't break console.error if telemetry fails originalConsoleError('[Console Instrumentation]: Telemetry error:', telemetryError.message); } };
231-259: Stack trace will not reflect the original caller.Similar to the
console.errorwrapper, the stack capture on line 246 creates a newError()inside the wrapper, so the stack trace will start from this instrumentation code rather than the original caller ofconsole.warn.Apply the same fix as suggested for
console.error:console.warn = function(...args) { + // Capture stack before calling original to preserve call site + const stack = (new Error()).stack; + // Call original console.warn first originalConsoleWarn.apply(console, args); // Send critical warnings to telemetry (7TV, SLO violations, etc.) try { const warnMessage = args.join(' '); const isCriticalWarning = warnMessage.includes('[7TV') || warnMessage.includes('[SLO') || warnMessage.includes('VIOLATION') || warnMessage.includes('CRITICAL'); if (isCriticalWarning && typeof window.app?.telemetry?.recordError === 'function') { const warning = new Error(`Warning: ${warnMessage}`); - warning.stack = (new Error()).stack; + warning.stack = stack; // Use pre-captured stack window.app.telemetry.recordError(warning, { source: 'console.warn', operation: 'console_warn_capture', severity: 'warning', arguments: args.length, timestamp: Date.now() }); } } catch (telemetryError) { // Don't break console.warn if telemetry fails } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/renderer/src/telemetry/webTracing.js(2 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**
📄 CodeRabbit inference engine (AGENTS.md)
**: Use electron-vite + React conventions instead of raw Electron patterns
When unsure, consult electron-vite/Electron/Vite docs (Context7) to confirm patterns
Omit empty sections in release notes
Files:
src/renderer/src/telemetry/webTracing.js
src/renderer/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/renderer/src/**/*.{ts,tsx,js,jsx}: Do not use direct Node APIs in the renderer
Use ipcRenderer.invoke('channel', payload) for request/response from renderer
Use import.meta.env.RENDERER_VITE_* for renderer-scoped config
Avoid bare KT_* env vars in the renderer; use RENDERER_VITE_KT_* instead
Files:
src/renderer/src/telemetry/webTracing.js
src/{renderer/src,preload}/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Resolve static assets via Vite URLs (new URL('./asset', import.meta.url))
Files:
src/renderer/src/telemetry/webTracing.js
src/{main,renderer/src}/**/*.{ts,tsx,js,jsx,mts,mjs,cjs}
📄 CodeRabbit inference engine (AGENTS.md)
For fire-and-forget events, use ipcRenderer.send and ipcMain.on with namespaced channels like 'app:settings:get'
Files:
src/renderer/src/telemetry/webTracing.js
src/{main,preload,renderer/src}/**/*.{ts,tsx,js,jsx,mts,mjs,cjs}
📄 CodeRabbit inference engine (AGENTS.md)
Use shared non-secret variables via import.meta.env.VITE_*
Files:
src/renderer/src/telemetry/webTracing.js
src/{telemetry,renderer/src/telemetry}/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/{telemetry,renderer/src/telemetry}/**/*.{ts,tsx,js,jsx}: Place OpenTelemetry instrumentation under src/telemetry/** and src/renderer/src/telemetry/**
Configure OTLP HTTP export to Grafana Cloud; read endpoints/keys from env with main-safe prefixes; do not expose secrets to renderer
Files:
src/renderer/src/telemetry/webTracing.js
!dist/**
📄 CodeRabbit inference engine (AGENTS.md)
Do not commit built installers/artifacts in dist/
Files:
src/renderer/src/telemetry/webTracing.js
🧬 Code graph analysis (1)
src/renderer/src/telemetry/webTracing.js (1)
src/telemetry/tracing.js (1)
telemetryEnabled(12-12)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test-builds (macos-latest)
- GitHub Check: test-builds (windows-latest)
🔇 Additional comments (2)
src/renderer/src/telemetry/webTracing.js (2)
281-285: LGTM!Console restoration logic is correct and consistent with the WebSocket restoration pattern above.
1305-1315: LGTM! Past concern addressed.The DEV-only test span is now properly guarded by
NODE_ENV === 'development'check, preventing any test emissions in production. This addresses the past review concern about synthetic test errors polluting production telemetry.
Implement progressive image loading for emotes to improve perceived performance and prevent layout shift during load. Changes: - Add useProgressiveEmoteLoading hook with 4 states (placeholder, loading, loaded, error) - Render fallback placeholders (2-char abbreviation) during load and on error - Smooth opacity transitions when images finish loading - SCSS styling for placeholder states with error indicator Impact: - Eliminates layout shift from async emote loading - Better UX with visual feedback during image load - Graceful degradation for failed emote loads
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/renderer/src/components/Cosmetics/Emote.jsx (2)
123-151: Risk of visual conflicts between inline styles and CSS classes.The image element's opacity is controlled both inline (line 148) and via CSS classes defined in Message.scss (lines 1060-1070). When
loadStateis'loaded', both mechanisms attempt to setopacity: 1, but during'loading'the inline style setsopacity: 0while the CSS.emote.loadingclass also setsopacity: 0. This redundancy is harmless but adds complexity.More critically, the
positionstyle switches from'absolute'to'static'when loaded (line 147). This approach may cause layout shifts if the placeholder is removed before the image's position changes, or if the placeholder and image have different dimensions.Consider consolidating opacity control to CSS only by removing inline opacity:
onLoad={handleImageLoad} onError={handleImageError} onLoadStart={handleImageLoadStart} style={{ - position: loadState === 'loaded' ? 'static' : 'absolute', - opacity: loadState === 'loaded' ? 1 : 0, - transition: 'opacity 0.2s ease-in-out' + position: loadState === 'loaded' ? 'static' : 'absolute' }} />The CSS classes already handle opacity transitions (Message.scss lines 1058-1066).
154-169: Consider progressive loading for overlaid emotes.Overlaid (zero-width) emotes (lines 155-169) don't use the progressive loading hook, meaning they may pop in abruptly or fail silently without placeholders. This creates an inconsistent user experience compared to the main emote.
If overlaid emotes are important to the visual experience, consider applying similar progressive loading logic to them. However, if they're decorative or rarely used, the current implementation is acceptable.
src/renderer/src/assets/styles/components/Chat/Message.scss (1)
1038-1054: Placeholder styles partially duplicate inline styles.The
.emote-placeholderCSS class (lines 1038-1054) duplicates several properties already defined inline in the JavaScriptplaceholderobject (Emote.jsx lines 14-25):backgroundColor,color,fontSize,borderRadius,display,alignItems,justifyContent, andfontFamily.However, the CSS adds a
borderproperty (line 1047) that's not present in the JS object, creating a minor inconsistency. When the placeholder<div>is rendered (Emote.jsx lines 125-131), it receives both inline styles from the JS object and the CSS class, with the inline styles taking precedence.For better maintainability, consolidate styling in one place:
Option 1 (preferred): Remove duplicate properties from the JS object and rely on CSS classes:
const placeholder = useMemo(() => { const placeholderWidth = type === "stv" ? (emote.width || 28) : 32; const placeholderHeight = type === "stv" ? (emote.height || 28) : 32; return { width: placeholderWidth, - height: placeholderHeight, - backgroundColor: '#2a2a2a', - display: 'inline-flex', - alignItems: 'center', - justifyContent: 'center', - borderRadius: '4px', - color: '#666', - fontSize: '10px', - fontFamily: 'monospace' + height: placeholderHeight }; }, [emote, type]);Option 2: Add the border to the JS object for consistency:
return { width: placeholderWidth, height: placeholderHeight, + border: '1px solid rgba(255, 255, 255, 0.1)', ... };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/renderer/src/assets/styles/components/Chat/Message.scss(1 hunks)src/renderer/src/components/Cosmetics/Emote.jsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**
📄 CodeRabbit inference engine (AGENTS.md)
**: Use electron-vite + React conventions instead of raw Electron patterns
When unsure, consult electron-vite/Electron/Vite docs (Context7) to confirm patterns
Omit empty sections in release notes
Files:
src/renderer/src/components/Cosmetics/Emote.jsxsrc/renderer/src/assets/styles/components/Chat/Message.scss
src/renderer/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/renderer/src/**/*.{ts,tsx,js,jsx}: Do not use direct Node APIs in the renderer
Use ipcRenderer.invoke('channel', payload) for request/response from renderer
Use import.meta.env.RENDERER_VITE_* for renderer-scoped config
Avoid bare KT_* env vars in the renderer; use RENDERER_VITE_KT_* instead
Files:
src/renderer/src/components/Cosmetics/Emote.jsx
src/{renderer/src,preload}/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Resolve static assets via Vite URLs (new URL('./asset', import.meta.url))
Files:
src/renderer/src/components/Cosmetics/Emote.jsx
src/{main,renderer/src}/**/*.{ts,tsx,js,jsx,mts,mjs,cjs}
📄 CodeRabbit inference engine (AGENTS.md)
For fire-and-forget events, use ipcRenderer.send and ipcMain.on with namespaced channels like 'app:settings:get'
Files:
src/renderer/src/components/Cosmetics/Emote.jsx
src/{main,preload,renderer/src}/**/*.{ts,tsx,js,jsx,mts,mjs,cjs}
📄 CodeRabbit inference engine (AGENTS.md)
Use shared non-secret variables via import.meta.env.VITE_*
Files:
src/renderer/src/components/Cosmetics/Emote.jsx
!dist/**
📄 CodeRabbit inference engine (AGENTS.md)
Do not commit built installers/artifacts in dist/
Files:
src/renderer/src/components/Cosmetics/Emote.jsxsrc/renderer/src/assets/styles/components/Chat/Message.scss
🧬 Code graph analysis (1)
src/renderer/src/components/Cosmetics/Emote.jsx (2)
src/renderer/src/components/Cosmetics/EmoteTooltip.jsx (1)
handleImageLoad(47-49)src/renderer/src/components/Cosmetics/Badges.jsx (3)
handleMouseMove(22-29)handleMouseMove(63-70)handleMouseMove(113-120)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test-builds (windows-latest)
- GitHub Check: test-builds (macos-latest)
🔇 Additional comments (1)
src/renderer/src/assets/styles/components/Chat/Message.scss (1)
1056-1072: LGTM! Progressive loading styles are well-structured.The
.emote-progressivestyles cleanly define the loading state transitions with appropriate opacity values and a smooth 0.2s transition. The state classes (.loading,.loaded,.error) provide clear visual feedback during the image loading lifecycle.As noted in the Emote.jsx review, there's redundant opacity control between these CSS classes and inline styles (Emote.jsx line 148), but the CSS approach here is preferable and should be retained while removing the inline opacity.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
utils/services/seventv/sharedStvWebSocket.js (1)
538-550: The isPersonalEmoteSet flag is still not used downstream.As noted in a previous review, the
isPersonalEmoteSetflag added toCustomEvent.detailis never read byhandleStvMessagein ChatProvider.jsx, which only destructurestypeandbody. The flag is not forwarded tohandleEmoteSetUpdate, which also ignores it.Update
handleStvMessage(around line 1808 in ChatProvider.jsx) to destructureisPersonalEmoteSetfromevent.detailand pass it tohandleEmoteSetUpdate. Then updatehandleEmoteSetUpdate(around line 2865 in ChatProvider.jsx) to accept and branch on this parameter.Based on learnings
src/renderer/src/providers/ChatProvider.jsx (1)
1810-1853: Cosmetic dedup still collapses aggregated payloads
Thecosmetic.createpath still derivesdedupKeyfrombody.object.data. For the aggregated payloads we emit fromsharedStvWebSocketthis object is missing, so the key becomescosmetic.create_undefined_unknownfor every aggregated batch. Just like before, the first event wins and everything that follows for the next 30 s is dropped, so real badge/paint updates never make it into the store. Please derive the key from the actual cosmetic IDs insidebody.cosmetics(iterate them) or disable dedup for that payload shape.
🧹 Nitpick comments (1)
utils/services/seventv/sharedStvWebSocket.js (1)
273-275: Consider gating these logs behind development mode.These subscription setup logs will appear in production. Since the PR focuses on performance optimization, consider wrapping them with
if (process.env.NODE_ENV === 'development')to reduce production noise.Apply this diff:
- console.log(`[Shared7TV]: Starting subscription to events for ${this.chatrooms.size} chatrooms`); + if (process.env.NODE_ENV === 'development') { + console.log(`[Shared7TV]: Starting subscription to events for ${this.chatrooms.size} chatrooms`); + } await this.subscribeToAllEvents(); - console.log(`[Shared7TV]: Completed subscription setup. Subscribed events: ${this.subscribedEvents.size}`); + if (process.env.NODE_ENV === 'development') { + console.log(`[Shared7TV]: Completed subscription setup. Subscribed events: ${this.subscribedEvents.size}`); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.vscode/launch.json(1 hunks)electron.vite.config.mjs(1 hunks)src/renderer/src/components/Cosmetics/Emote.jsx(2 hunks)src/renderer/src/providers/ChatProvider.jsx(16 hunks)src/renderer/src/providers/CosmeticsProvider.jsx(5 hunks)utils/services/seventv/sharedStvWebSocket.js(6 hunks)
✅ Files skipped from review due to trivial changes (1)
- .vscode/launch.json
🚧 Files skipped from review as they are similar to previous changes (1)
- src/renderer/src/components/Cosmetics/Emote.jsx
🧰 Additional context used
📓 Path-based instructions (7)
**
📄 CodeRabbit inference engine (AGENTS.md)
**: Use electron-vite + React conventions instead of raw Electron patterns
When unsure, consult electron-vite/Electron/Vite docs (Context7) to confirm patterns
Omit empty sections in release notes
Files:
src/renderer/src/providers/ChatProvider.jsxelectron.vite.config.mjsutils/services/seventv/sharedStvWebSocket.jssrc/renderer/src/providers/CosmeticsProvider.jsx
src/renderer/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/renderer/src/**/*.{ts,tsx,js,jsx}: Do not use direct Node APIs in the renderer
Use ipcRenderer.invoke('channel', payload) for request/response from renderer
Use import.meta.env.RENDERER_VITE_* for renderer-scoped config
Avoid bare KT_* env vars in the renderer; use RENDERER_VITE_KT_* instead
Files:
src/renderer/src/providers/ChatProvider.jsxsrc/renderer/src/providers/CosmeticsProvider.jsx
src/{renderer/src,preload}/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Resolve static assets via Vite URLs (new URL('./asset', import.meta.url))
Files:
src/renderer/src/providers/ChatProvider.jsxsrc/renderer/src/providers/CosmeticsProvider.jsx
src/{main,renderer/src}/**/*.{ts,tsx,js,jsx,mts,mjs,cjs}
📄 CodeRabbit inference engine (AGENTS.md)
For fire-and-forget events, use ipcRenderer.send and ipcMain.on with namespaced channels like 'app:settings:get'
Files:
src/renderer/src/providers/ChatProvider.jsxsrc/renderer/src/providers/CosmeticsProvider.jsx
src/{main,preload,renderer/src}/**/*.{ts,tsx,js,jsx,mts,mjs,cjs}
📄 CodeRabbit inference engine (AGENTS.md)
Use shared non-secret variables via import.meta.env.VITE_*
Files:
src/renderer/src/providers/ChatProvider.jsxsrc/renderer/src/providers/CosmeticsProvider.jsx
!dist/**
📄 CodeRabbit inference engine (AGENTS.md)
Do not commit built installers/artifacts in dist/
Files:
src/renderer/src/providers/ChatProvider.jsxelectron.vite.config.mjsutils/services/seventv/sharedStvWebSocket.jssrc/renderer/src/providers/CosmeticsProvider.jsx
electron.vite.config.mjs
📄 CodeRabbit inference engine (AGENTS.md)
If bare KT_* must be exposed, add envPrefix override in electron.vite.config.mjs
Files:
electron.vite.config.mjs
🧬 Code graph analysis (3)
src/renderer/src/providers/ChatProvider.jsx (3)
utils/services/seventv/stvAPI.js (1)
sendUserPresence(100-127)src/renderer/src/providers/CosmeticsProvider.jsx (2)
span(11-11)startSpan(7-19)src/preload/index.js (1)
personalEmoteSets(112-112)
utils/services/seventv/sharedStvWebSocket.js (1)
src/renderer/src/providers/CosmeticsProvider.jsx (1)
INVALID_7TV_NULL_ID(21-21)
src/renderer/src/providers/CosmeticsProvider.jsx (4)
src/renderer/src/providers/ChatProvider.jsx (4)
getRendererTracer(48-49)startSpan(51-67)tracer(53-53)span(55-55)src/renderer/src/utils/MessageParser.jsx (4)
getRendererTracer(7-8)startSpan(12-42)tracer(28-28)span(30-30)utils/services/seventv/sharedStvWebSocket.js (2)
tracer(8-8)INVALID_7TV_NULL_ID(5-5)src/renderer/src/components/Cosmetics/Badges.jsx (2)
badgeUrl(72-72)badgeUrl(122-122)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test-builds (windows-latest)
- GitHub Check: test-builds (macos-latest)
🔇 Additional comments (5)
utils/services/seventv/sharedStvWebSocket.js (4)
67-124: LGTM! Clean emote set subscription management.The
updateChatroommethod correctly handles emote set ID changes by unsubscribing from old events and subscribing to new ones. The validation of old emote set IDs before unsubscribing and the development-mode logging are appropriate.
503-523: LGTM! Good use of debug flag.The debug logging controlled by
VITE_DEBUG_7TV_WSis well-implemented and will help with troubleshooting without impacting production performance.
552-590: LGTM! Good observability improvements.The addition of the
cosmetic.deletecase and the default telemetry span for unhandled events improve observability and completeness.
598-660: LGTM! Enhanced event routing with personal emote set support.The signature change from
(body, type)to(type, body)is consistent with the call site at line 518. The addition of type validation, personal emote set handling (kind === 3), and comprehensive debug logging are all improvements. The fallback logic for multiple ID field names (id,object_id,emote_set_id) is good defensive programming.electron.vite.config.mjs (1)
75-75: Scope source maps to non-production buildsSource maps are enabled unconditionally (
sourcemap: true), so your production bundles will ship.mapfiles. If you only need them for local debugging, wrap this in aNODE_ENV === 'development'check or use the'hidden'option to avoid exposing full maps in production.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/renderer/src/providers/ChatProvider.jsx (3)
2972-3021: Consider simplifying dual deduplication strategy.The channel emote update logic uses both a detailed deduplication key (full signature) and a simple deduplication key (just set ID with 5s TTL). While this provides robust protection against burst updates, the dual-tier approach adds complexity. The simple dedup is only applied when the set "actually exists" in chatrooms, but this check iterates all chatrooms for every update.
Consider consolidating to a single deduplication strategy using a stable signature with a shorter TTL (e.g., 5-10 seconds) to balance simplicity and effectiveness.
3227-3396: Personal emote updates lack user-facing notifications.Channel emote updates add a message to chat (lines 3154-3170) when emotes are added/removed/updated, but personal emote set updates processed by
handlePersonalEmoteSetUpdatedo not produce any user-visible notification. Users may not realize their personal emotes have changed unless they check the emote picker.Consider adding a subtle notification or toast when personal emote sets are updated to improve user awareness.
3398-3423: Implement personal emote set entitlement handler.The stub for
handlePersonalEmoteSetEntitlementis incomplete and marked with a TODO referencing GitHub issue #48. This handler is critical for rendering other users' personal emotes in chat when they gain/lose access to personal emote sets via entitlement events.Track implementation progress and ensure this is completed to fully support personal emote rendering across all users.
Do you want me to generate a skeleton implementation based on the existing cosmetic entitlement handlers, or open an issue to track this work?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/renderer/src/providers/ChatProvider.jsx(16 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**
📄 CodeRabbit inference engine (AGENTS.md)
**: Use electron-vite + React conventions instead of raw Electron patterns
When unsure, consult electron-vite/Electron/Vite docs (Context7) to confirm patterns
Omit empty sections in release notes
Files:
src/renderer/src/providers/ChatProvider.jsx
src/renderer/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/renderer/src/**/*.{ts,tsx,js,jsx}: Do not use direct Node APIs in the renderer
Use ipcRenderer.invoke('channel', payload) for request/response from renderer
Use import.meta.env.RENDERER_VITE_* for renderer-scoped config
Avoid bare KT_* env vars in the renderer; use RENDERER_VITE_KT_* instead
Files:
src/renderer/src/providers/ChatProvider.jsx
src/{renderer/src,preload}/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Resolve static assets via Vite URLs (new URL('./asset', import.meta.url))
Files:
src/renderer/src/providers/ChatProvider.jsx
src/{main,renderer/src}/**/*.{ts,tsx,js,jsx,mts,mjs,cjs}
📄 CodeRabbit inference engine (AGENTS.md)
For fire-and-forget events, use ipcRenderer.send and ipcMain.on with namespaced channels like 'app:settings:get'
Files:
src/renderer/src/providers/ChatProvider.jsx
src/{main,preload,renderer/src}/**/*.{ts,tsx,js,jsx,mts,mjs,cjs}
📄 CodeRabbit inference engine (AGENTS.md)
Use shared non-secret variables via import.meta.env.VITE_*
Files:
src/renderer/src/providers/ChatProvider.jsx
!dist/**
📄 CodeRabbit inference engine (AGENTS.md)
Do not commit built installers/artifacts in dist/
Files:
src/renderer/src/providers/ChatProvider.jsx
🧬 Code graph analysis (1)
src/renderer/src/providers/ChatProvider.jsx (3)
utils/services/seventv/stvAPI.js (1)
sendUserPresence(100-127)src/renderer/src/providers/CosmeticsProvider.jsx (2)
useCosmeticsStore(143-425)span(11-11)src/preload/index.js (1)
personalEmoteSets(112-112)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test-builds (windows-latest)
🔇 Additional comments (7)
src/renderer/src/providers/ChatProvider.jsx (7)
390-392: LGTM! Deduplication state properly initialized.The new state fields for tracking recent updates are correctly initialized as empty collections and align with the deduplication patterns used throughout the file.
540-564: Return value change enables batching optimization.The method now returns the update timestamp when successful or
nullwhen skipped, allowing the caller to track which chatrooms received presence updates. This supports the batched logging implementation at lines 4248-4278.
1419-1439: Global cosmetic routing correctly centralizes processing.Cosmetic and entitlement events without a specific
chatroomIdare now handled globally (once) by passingnullas the chatroom ID. This prevents duplicate processing across multiple chatrooms and aligns with the PR's performance optimization goals.
2921-2952: Personal emote deduplication correctly uses stable keys.The deduplication key now uses
JSON.stringifyof the update signature (pulled/pushed/updated emotes), which is stable and reusable within the TTL window. This correctly addresses the issue flagged in the previous review whereDate.now()made every key unique.
1861-1916: Cosmetic and entitlement event routing is correct.The handlers properly integrate with
CosmeticsProvidermethods (addCosmetic,removeCosmetic,addUserStyle,removeUserStyle) and route events byobjectKind. Telemetry spans for unhandled kinds provide good observability for future extension points.
4248-4278: Batched presence updates reduce logging noise and improve performance.Processing presence updates in batches of 3 with 1ms delays between batches prevents blocking the event loop. The consolidated logging (one summary log per interval instead of per-chatroom) significantly reduces console noise while maintaining visibility into the presence update process.
858-862: No active usages ofconnectToStvWebSocketremain. Both occurrences in ChatProvider.jsx are already commented out.
Add hash-based fallback for cosmetic.create events that lack unique IDs (e.g., aggregated payloads). Previously, all such events fell back to 'unknown', causing collisions. Now hashes the payload to generate a unique dedupe key. Also adds debug logging to track dedupe behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/renderer/src/providers/ChatProvider.jsx (2)
1432-1445: Review: Broadcasting non-cosmetic events to all chatrooms may be inefficient.When a 7TV event arrives without a
chatroomId, cosmetic events are correctly handled once globally, but non-cosmetic events are broadcast to every chatroom. This could cause:
- Unnecessary processing overhead if many chatrooms are open
- Potential duplicate event handling
- Events intended for one room processed by all rooms
Consider adding telemetry to monitor which non-cosmetic event types arrive without
chatroomId, and whether they truly need broadcasting vs. being safely ignored.Add telemetry to identify non-cosmetic events without chatroomId:
} else { - // Broadcast to all chatrooms if no specific chatroom (for non-cosmetic events) + // Log non-cosmetic events without chatroomId for telemetry + window.app?.telemetry?.recordError?.(new Error('STV event without chatroomId'), { + operation: 'handleStvMessage_broadcast', + event_type: type, + body_keys: Object.keys(body || {}).join(','), + severity: 'info' + }); + + // Broadcast to all chatrooms if no specific chatroom (for non-cosmetic events) chatrooms.forEach(chatroom => { get().handleStvMessage(chatroom.id, event.detail); }); }
3433-3458: Reminder: Implement personal emote set entitlement handler.The stub implementation is appropriately documented with a TODO referencing GitHub issue #48. The comments clearly outline the intended behavior for both current user and other users' entitlements.
Would you like me to open a GitHub issue to track this implementation task if one doesn't exist yet?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/renderer/src/providers/ChatProvider.jsx(17 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**
📄 CodeRabbit inference engine (AGENTS.md)
**: Use electron-vite + React conventions instead of raw Electron patterns
When unsure, consult electron-vite/Electron/Vite docs (Context7) to confirm patterns
Omit empty sections in release notes
Files:
src/renderer/src/providers/ChatProvider.jsx
src/renderer/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/renderer/src/**/*.{ts,tsx,js,jsx}: Do not use direct Node APIs in the renderer
Use ipcRenderer.invoke('channel', payload) for request/response from renderer
Use import.meta.env.RENDERER_VITE_* for renderer-scoped config
Avoid bare KT_* env vars in the renderer; use RENDERER_VITE_KT_* instead
Files:
src/renderer/src/providers/ChatProvider.jsx
src/{renderer/src,preload}/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Resolve static assets via Vite URLs (new URL('./asset', import.meta.url))
Files:
src/renderer/src/providers/ChatProvider.jsx
src/{main,renderer/src}/**/*.{ts,tsx,js,jsx,mts,mjs,cjs}
📄 CodeRabbit inference engine (AGENTS.md)
For fire-and-forget events, use ipcRenderer.send and ipcMain.on with namespaced channels like 'app:settings:get'
Files:
src/renderer/src/providers/ChatProvider.jsx
src/{main,preload,renderer/src}/**/*.{ts,tsx,js,jsx,mts,mjs,cjs}
📄 CodeRabbit inference engine (AGENTS.md)
Use shared non-secret variables via import.meta.env.VITE_*
Files:
src/renderer/src/providers/ChatProvider.jsx
!dist/**
📄 CodeRabbit inference engine (AGENTS.md)
Do not commit built installers/artifacts in dist/
Files:
src/renderer/src/providers/ChatProvider.jsx
🧬 Code graph analysis (1)
src/renderer/src/providers/ChatProvider.jsx (3)
utils/services/seventv/stvAPI.js (1)
sendUserPresence(100-127)src/renderer/src/providers/CosmeticsProvider.jsx (1)
startSpan(7-19)src/preload/index.js (1)
personalEmoteSets(112-112)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test-builds (windows-latest)
- GitHub Check: test-builds (macos-latest)
🔇 Additional comments (8)
src/renderer/src/providers/ChatProvider.jsx (8)
37-46: LGTM! Clean implementation of deterministic hash.The
hashCodehelper provides a simple, deterministic hash for creating deduplication keys from payload strings. The bit manipulation and base36 conversion are appropriate for this use case.
401-403: LGTM! Appropriate deduplication state structures.The new state trackers use appropriate data structures for their purposes:
Setfor personal/channel update keys (simple presence checking)Mapfor cosmetic events (needs timestamps for TTL-based cleanup)
1822-1888: LGTM! Deduplication logic properly handles aggregated payloads.The cosmetic event deduplication now correctly handles cases where IDs are unavailable by using a payload hash fallback (lines 1843-1853). This addresses the previous review concern about aggregated payloads causing all events to deduplicate under
"cosmetic.create_undefined_undefined".The 30-second deduplication window with 60-second cleanup is appropriate for this use case.
Based on learnings (previous review comments indicated concern about aggregated payloads lacking IDs).
3262-3431: LGTM! Personal emote set update handler is well-implemented.The function correctly processes personal emote set updates globally (not per-chatroom), mirroring the logic used for channel emotes. Key strengths:
- Handles all update types (pulled/pushed/updated)
- Updates both state and localStorage
- Comprehensive telemetry tracking
- Proper span lifecycle management
Note: Deduplication is handled by the caller (
handleEmoteSetUpdate), which is the correct design.
4283-4313: LGTM! Batch processing improves presence update performance.The presence update logic now processes chatrooms in small batches (3 at a time) with 1ms delays between batches. This prevents blocking the main thread and reduces logging noise by only emitting a summary after all batches complete.
The recursive batch processing with closure-based state is clean and efficient.
551-575: sendPresenceUpdate return value handling verified. The sole caller at line 4296 checksif (sentAt)before updating rooms, so behavior remains correct.
869-873: No remaining active calls to connectToStvWebSocket
Only commented references at src/renderer/src/providers/ChatProvider.jsx:1554 and 2185—deprecation complete.
2935-3260: recentSimpleChannelUpdates is safely lazily initialized
All usages guard against undefined (get().recentSimpleChannelUpdates || new Set()) and the finalset(...)call ensures it's added to state on first run. No initial-state declaration needed.
Add trace span to monitor non-cosmetic 7TV events that arrive without a chatroomId and get broadcast to all chatrooms. This helps identify potential inefficiencies and whether certain event types need different routing logic. Tracks event type, chatroom count, and body preview for analysis.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/renderer/src/providers/ChatProvider.jsx (1)
3035-3055: Verify cleanup ofrecentSimpleChannelUpdatesentries.The code adds entries to
recentSimpleChannelUpdates(line 3048) and schedules cleanup after 5 seconds (lines 3049-3054), but the Set is never bounded. If emote updates arrive faster than cleanup executes, this Set could grow indefinitely.Consider adding a size limit similar to the other dedup Sets:
// Mark as recently processed (expires in 5 seconds) recentSimpleUpdates.add(`simple_${setId}`); + + // Prevent unbounded growth + if (recentSimpleUpdates.size > 500) { + const updates = Array.from(recentSimpleUpdates); + updates.slice(0, updates.length - 500).forEach(key => recentSimpleUpdates.delete(key)); + } + setTimeout(() => { const currentState = get(); const updates = currentState.recentSimpleChannelUpdates || new Set(); updates.delete(`simple_${setId}`); set({ recentSimpleChannelUpdates: updates }); }, 5000);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/renderer/src/providers/ChatProvider.jsx(17 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**
📄 CodeRabbit inference engine (AGENTS.md)
**: Use electron-vite + React conventions instead of raw Electron patterns
When unsure, consult electron-vite/Electron/Vite docs (Context7) to confirm patterns
Omit empty sections in release notes
Files:
src/renderer/src/providers/ChatProvider.jsx
src/renderer/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/renderer/src/**/*.{ts,tsx,js,jsx}: Do not use direct Node APIs in the renderer
Use ipcRenderer.invoke('channel', payload) for request/response from renderer
Use import.meta.env.RENDERER_VITE_* for renderer-scoped config
Avoid bare KT_* env vars in the renderer; use RENDERER_VITE_KT_* instead
Files:
src/renderer/src/providers/ChatProvider.jsx
src/{renderer/src,preload}/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Resolve static assets via Vite URLs (new URL('./asset', import.meta.url))
Files:
src/renderer/src/providers/ChatProvider.jsx
src/{main,renderer/src}/**/*.{ts,tsx,js,jsx,mts,mjs,cjs}
📄 CodeRabbit inference engine (AGENTS.md)
For fire-and-forget events, use ipcRenderer.send and ipcMain.on with namespaced channels like 'app:settings:get'
Files:
src/renderer/src/providers/ChatProvider.jsx
src/{main,preload,renderer/src}/**/*.{ts,tsx,js,jsx,mts,mjs,cjs}
📄 CodeRabbit inference engine (AGENTS.md)
Use shared non-secret variables via import.meta.env.VITE_*
Files:
src/renderer/src/providers/ChatProvider.jsx
!dist/**
📄 CodeRabbit inference engine (AGENTS.md)
Do not commit built installers/artifacts in dist/
Files:
src/renderer/src/providers/ChatProvider.jsx
🧬 Code graph analysis (1)
src/renderer/src/providers/ChatProvider.jsx (4)
utils/services/seventv/stvAPI.js (1)
sendUserPresence(100-127)src/renderer/src/providers/CosmeticsProvider.jsx (2)
span(11-11)startSpan(7-19)src/renderer/src/pages/ChatPage.jsx (4)
span(53-53)startSpan(49-65)endSpanOk(67-70)endSpanError(72-75)src/preload/index.js (1)
personalEmoteSets(112-112)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test-builds (windows-latest)
🔇 Additional comments (4)
src/renderer/src/providers/ChatProvider.jsx (4)
37-46: LGTM: Hash function implementation is correct.The
hashCodehelper uses a standard string hashing algorithm (similar to djb2) with bitwise operations for efficiency. The conversion to base36 provides shorter, URL-safe strings suitable for dedup keys.
1832-1898: Improved deduplication logic addresses past issues.The cosmetic event deduplication now correctly handles aggregated payloads by falling back to payload hashing when IDs are unavailable. This prevents all aggregated events from colliding under a single
'unknown'key.Key improvements:
- Sentinel ID exclusion (line 1842)
- Multiple ID sources checked (rawId, refId, body.id)
- Payload hash fallback (lines 1853-1863) prevents collisions
- Proper TTL cleanup (lines 1891-1895)
2970-2997: Personal emote set deduplication correctly fixed.The deduplication key now uses a stable signature based on
body.idand the update content (line 2972-2975), which addresses the past review issue whereDate.now()made every key unique. This ensures duplicate personal emote set updates are properly skipped within the processing window.
4294-4323: LGTM: Presence update batching is well-implemented.The batching approach (groups of 3, line 4300) with 1ms delays (line 4316) effectively prevents blocking the main thread while processing multiple chatrooms. The logging is appropriately simplified to only show a summary after all batches complete (lines 4318-4319).
The function also correctly returns
nullearly when no 7TV ID is provided (line 554) or no auth tokens are available (line 560), preventing unnecessary processing.
Fix undefined variable error by accessing chatrooms from store state via get().chatrooms. Also changed .size to .length (array property) and added optional chaining for safety.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/renderer/src/providers/ChatProvider.jsx (2)
3018-3067: Consider simplifying the triple-layered channel deduplication.The channel update path has three deduplication mechanisms:
- Full body signature dedup (
recentChannelUpdates, line 3019)- Set existence check (
setActuallyExists, lines 3032-3034)- Simple set ID + 5s TTL dedup (
recentSimpleChannelUpdates, lines 3041-3055)This complexity may be unnecessary. Consider:
- Using only the full body signature dedup for correctness
- Adding the 5s TTL simple dedup as an additional fast-path guard
- Removing the
setActuallyExistscheck unless it serves a specific purposeThe current implementation is functional but adds cognitive overhead and potential maintenance burden.
3444-3469: Stub implementation flagged for completion.The function is a stub with a TODO comment referencing GitHub issue #48. While the field extraction logic is correct, the actual handling of entitlement create/delete events is not implemented. Ensure issue #48 is tracked and prioritized to complete this functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/renderer/src/providers/ChatProvider.jsx(17 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**
📄 CodeRabbit inference engine (AGENTS.md)
**: Use electron-vite + React conventions instead of raw Electron patterns
When unsure, consult electron-vite/Electron/Vite docs (Context7) to confirm patterns
Omit empty sections in release notes
Files:
src/renderer/src/providers/ChatProvider.jsx
src/renderer/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/renderer/src/**/*.{ts,tsx,js,jsx}: Do not use direct Node APIs in the renderer
Use ipcRenderer.invoke('channel', payload) for request/response from renderer
Use import.meta.env.RENDERER_VITE_* for renderer-scoped config
Avoid bare KT_* env vars in the renderer; use RENDERER_VITE_KT_* instead
Files:
src/renderer/src/providers/ChatProvider.jsx
src/{renderer/src,preload}/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Resolve static assets via Vite URLs (new URL('./asset', import.meta.url))
Files:
src/renderer/src/providers/ChatProvider.jsx
src/{main,renderer/src}/**/*.{ts,tsx,js,jsx,mts,mjs,cjs}
📄 CodeRabbit inference engine (AGENTS.md)
For fire-and-forget events, use ipcRenderer.send and ipcMain.on with namespaced channels like 'app:settings:get'
Files:
src/renderer/src/providers/ChatProvider.jsx
src/{main,preload,renderer/src}/**/*.{ts,tsx,js,jsx,mts,mjs,cjs}
📄 CodeRabbit inference engine (AGENTS.md)
Use shared non-secret variables via import.meta.env.VITE_*
Files:
src/renderer/src/providers/ChatProvider.jsx
!dist/**
📄 CodeRabbit inference engine (AGENTS.md)
Do not commit built installers/artifacts in dist/
Files:
src/renderer/src/providers/ChatProvider.jsx
🧬 Code graph analysis (1)
src/renderer/src/providers/ChatProvider.jsx (3)
utils/services/seventv/stvAPI.js (1)
sendUserPresence(100-127)src/renderer/src/providers/CosmeticsProvider.jsx (2)
span(11-11)startSpan(7-19)src/preload/index.js (1)
personalEmoteSets(112-112)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test-builds (macos-latest)
- GitHub Check: test-builds (windows-latest)
🔇 Additional comments (6)
src/renderer/src/providers/ChatProvider.jsx (6)
37-46: LGTM! Clean deterministic hash implementation.The hash function provides deterministic keys for deduplication without payload IDs. The bitwise operations ensure 32-bit integer conversion, and base36 encoding keeps keys compact.
550-575: LGTM! Explicit return values improve clarity.The function now returns explicit
nullfor early exits and the timestamp on success, which improves clarity and enables the caller to track which updates were sent.
1430-1461: LGTM! Global event routing and past scope issue resolved.The handler correctly routes global cosmetic/entitlement events to
handleStvMessagewithnullchatroomId, avoiding duplicate processing across chatrooms. The previously flaggedchatroomsscope issue is now resolved (line 1443).
1833-1899: LGTM! Aggregated payload deduplication issue resolved.The deduplication logic now correctly handles aggregated cosmetic payloads without IDs by using
hashCodeto generate unique keys (lines 1854-1864), addressing the past review concern about collisions. The TTL-based cleanup prevents unbounded Map growth.
3273-3442: LGTM! Clean implementation of global personal emote set updates.The function correctly processes personal emote set updates globally, transforms 7TV structures, updates state and localStorage, and records detailed telemetry. The logic mirrors the channel emote update flow with appropriate adaptations.
4294-4325: LGTM! Efficient batching prevents main thread blocking.The presence update logic now processes chatrooms in small batches (3 at a time) with 1ms delays between batches, preventing main thread blocking. The local store reference improves efficiency, and the simplified logging reduces noise.
) * Fix cross-platform icon compatibility - Add cross-platform icon handling for Linux/macOS support - Convert Windows .ico to .png for non-Windows platforms - Update tray and thumbar icons to use platform-appropriate format Fixes application crashes on Linux due to unsupported .ico format 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix(7tv): null-guard channel avatar in EmoteDialogs to prevent includes on undefined. Fixes KickTalkOrg#46 --------- Co-authored-by: Claude <noreply@anthropic.com>
1 participant
Summary
.find()operations with O(1)Map.get()lookupsProblem
After the recent 7TV styling bug fix, the app became laggy. Investigation revealed performance bottlenecks in cosmetics lookups where every message render was calling
.find()on potentially large arrays of badges and paints, creating O(n × messages) complexity.Solution
cosmeticsLookupstate withbadgeMapandpaintMaphashmaps for O(1) accessaddCosmetics()to populate lookup maps when cosmetics are loaded.find()calls withMap.get()ingetUserStyle,getUserBadge, andgetUserPaintPerformance Impact
Test Plan
Risk Assessment
Low Risk - The changes maintain the exact same API and functionality, only optimizing the internal lookup mechanism. Additional performance optimizations may follow.
Summary by CodeRabbit
Performance
New Features
Refactor
Bug Fixes
Chores